-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: refactor parameter constructor #455
fix: refactor parameter constructor #455
Conversation
Codecov ReportPatch and project coverage have no change.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #455 +/- ##
=======================================
Coverage 81.75% 81.75%
=======================================
Files 37 37
Lines 2006 2006
=======================================
Hits 1640 1640
Misses 366 366
☔ View full report in Codecov by Sentry. |
Typically the "_m" refers to a
class member. So the refactor should probably be more like:
class Parameter {
public:
double value_m; /**< initial value of the parameter*/
double min_m =
std::numeric_limits<double>::min(); /**< min value of the parameter*/
double max_m =
std::numeric_limits<double>::max(); /**< max value of the parameter*/
bool is_random_effect_m = false; /**< Is the parameter a random effect
parameter? Default value is false.*/
bool estimated_m =
false; /**< Is the parameter estimated? Default value is false.*/
/**
* @brief Constructor for initializing Parameter.
* @details Inputs include value, min, max, estimated.
*/
Parameter(double value, double min, double max, bool estimated)
: value_m(value), min_m(min), max_m(max), estimated_m(estimated) {}
/**
* @brief Constructor for initializing Parameter.
* @details Inputs include value.
*/
Parameter(double value) { this->value_m = value; }
/**
* @brief Constructor for initializing Parameter.
* @details Set value to 0 when there is no input value.
*/
Parameter() { this->value_m = 0; }
};
|
6ee5ab6
to
3035bd4
Compare
Thank you, @msupernaw for your suggestions. There was a display issue with your previous comment, but I've updated it. In the revised code, "_m" as a prefix for members of the Parameter class has been used. Additionally, the relevant code sections within the selectivity, recruitment, maturity, and TMB interfaces have been updated. Do we need to consider adding "interface_" as a prefix to those members (see issue #411)? If it becomes necessary during the work on issue #411 , we can implement this change at that time. |
Instructions for code reviewerHello reviewer, thanks for taking the time to review this PR!
Checklist
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great!
* fix: refactor parameter constructor * refactor: update members in the Parameter class
What is the feature?
How have you implemented the solution?
Updated the code to below:
Does the PR impact any other area of the project?
NA
How to test this change
All tests have passed. see GHA logs here: https://github.com/NOAA-FIMS/FIMS/actions?query=branch%3A218-refactor-refactor-values-min-max-and-estimated-from-the-parameter-constructor
Developer pre-PR checklist